Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: duplicate entries in search query #3289

Merged

Conversation

anshpathania7
Copy link
Contributor

What

  • added trimming of query before performing operations to avoid duplications due to whitespaces

Fixes bug(s)

@anshpathania7 anshpathania7 requested a review from a team as a code owner November 9, 2022 15:12
@github-actions github-actions bot added the 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… label Nov 9, 2022
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As easy as that! Thank you @anshpathania7!

Not 100% related: your commit history is a bit messy - like the 11 commits needed for this PR. Please clean this before your next PR.

@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2022

Codecov Report

Merging #3289 (f4407fc) into develop (b29c075) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #3289      +/-   ##
===========================================
- Coverage    10.50%   10.50%   -0.01%     
===========================================
  Files          249      249              
  Lines        12296    12297       +1     
===========================================
  Hits          1292     1292              
- Misses       11004    11005       +1     
Impacted Files Coverage Δ
...ackages/smooth_app/lib/pages/scan/search_page.dart 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@anshpathania7
Copy link
Contributor Author

Yes yes, I'll definitely clean up commits before further PRs :D

@@ -19,7 +19,8 @@ void _performSearch(
String query, {
EditProductQueryCallback? editProductQueryCallback,
}) {
if (query.trim().isEmpty) {
query = query.trim();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious , we aren't handling the "redbull" and "Redbull" as two different things here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AshAman999 The fix was only about spaces, not lower / upper case.
"redbull" and "Redbull" will be considered as different.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're going to handle that case or leave it as it is for now ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AshAman999 I think "redbull" and "Redbull", both should be treated as separate keywords as on UI side they don't look same (also There could be some brands having capital keywords?).

In search history, "redbull" and " redbull " were treated as separate keywords but to user they would look same (because couldn't tell difference between whitespaces), hence duplicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pops a question in my mind that should there be a some sort of suggestion in search box ? if user inputs something like "REdbUll" or "r ed bul l", then should there be suggestion for correct word?

Maybe this could be a feature?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah some kind of local autocomplete would make sense. Though as far as I can remember we only store 5 or so last searches atm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you guys have suggestions beyond this PR feel free to open a new issue, so that we can merge this successful PR first.
Seeing several times the exact same text (e.g. "redbull") (modulo the spaces) was a bit problematic, and that's what the PR was about. Done.

@monsieurtanuki monsieurtanuki merged commit 768a04a into openfoodfacts:develop Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion…
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants